-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(JOML): migrate SimpleFarming #101
Conversation
- clean up code - fix test cases PR: Terasology/ModuleTestingEnvironment#27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, have a few comments on this one 🙃 Overall looks fine, I'm just wondering how much effort we should put into setting all the interfaces straight to an idiomatic use of JOML…?
src/test/java/org/terasology/simpleFarming/systems/PlantAuthoritySystemTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/terasology/simpleFarming/systems/PlantAuthoritySystemTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/terasology/simpleFarming/systems/PlantAuthoritySystemTest.java
Outdated
Show resolved
Hide resolved
Assertions.assertTrue(dropSpy.getEntityRefs().stream().anyMatch(k -> k.getParentPrefab().getName().equals(component.produce))); | ||
|
||
// check if bush is at max growth state | ||
assertEquals(component.growthStages.size() - 2, component.currentStage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the max growth stage is one state before the final growth state? This is confusing 🙄
Block belowBlock = worldProvider.getBlock(position.add(0,-1,0)); | ||
position.add(0,1,0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can already see how error prone this is 🙈
src/main/java/org/terasology/simpleFarming/systems/PlantAuthoritySystem.java
Outdated
Show resolved
Hide resolved
@@ -298,11 +288,11 @@ public void onPlantDestroyed(DoDestroyPlant event, EntityRef entity, BushDefinit | |||
* | |||
* @param bushComponent the bush component of the entity | |||
*/ | |||
private void onBushDestroyed(Vector3i position, EntityRef bush, BushDefinitionComponent bushComponent) { | |||
private void onBushDestroyed(Vector3ic position, EntityRef bush, BushDefinitionComponent bushComponent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…itySystem.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
…itySystemTest.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
…itySystemTest.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
…itySystemTest.java Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>
PR: add relevance check to avoid creating additional entity ModuleTestingEnvironment#27